Repository navigation
Rewrite ecs core - #74
Conversation
|
I need to stop.. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
7b8a8ea to
497e47c
Compare
left a comment
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| try await Self.processBatch(batch, state: state, operation: operation) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Parallel forEach silently swallows task errors
The forEach method uses await withThrowingTaskGroup without try, while the analogous map method correctly uses try await withThrowingTaskGroup and consumes results with for try await. When child tasks throw errors in forEach, those errors are silently lost because the task group body doesn't throw directly and results aren't consumed. This creates an inconsistency where map properly propagates errors but forEach does not.
| try await Self.processBatch(batch, state: state, operation: operation) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Parallel forEach silently drops errors from tasks
The forEach method uses await withThrowingTaskGroup without try, and doesn't iterate over task results. When a child task throws an error (e.g., when operation throws), the error is not propagated to the caller. Compare this to the map method on lines 71-83, which correctly uses try await withThrowingTaskGroup and iterates with for try await batchResults in group. The forEach implementation will silently swallow any errors thrown by the parallel operations, making debugging extremely difficult and potentially leaving the system in an inconsistent state.
| uses: maxim-lobanov/setup-xcode@v1.6.0 | ||
| with: | ||
| xcode-version: '16.2.0' | ||
| xcode-version: '26.1.1' |
There was a problem hiding this comment.
Bug: Invalid macOS and Xcode versions in CI workflows
The GitHub Actions workflow configuration specifies macos-26 runner and xcode-version: '26.1.1', which do not exist. As of late 2024, the latest available macOS runner is macos-15 (Sequoia) and the latest Xcode is 16.x. These invalid version references will cause all CI builds to fail immediately since GitHub cannot provision non-existent runner images.
Additional Locations (1)
| self.addedEntities.insert(entity.id) | ||
| if entity.id != Entity.notAllocatedId { | ||
| entities.addNotAllocatedEntity(entity) | ||
| } |
There was a problem hiding this comment.
Bug: Inverted condition prevents entity ID allocation
The condition entity.id != Entity.notAllocatedId is inverted. When an entity is created with the public Entity(name:) constructor (which sets id = notAllocatedId) and added directly via world.addEntity(), the condition evaluates to false, so addNotAllocatedEntity is never called. This leaves the entity with an invalid ID of -25102018. The condition should be entity.id == Entity.notAllocatedId to properly allocate an ID for unallocated entities.
| xcode_version: ['16.2', '16.3'] | ||
| runs-on: macos-15 | ||
| xcode_version: ['26.1.1'] | ||
| runs-on: macos-26 |
There was a problem hiding this comment.
Bug: Invalid CI runner and Xcode versions will break builds
The CI workflows were changed to use macos-26 runner and xcode-version: '26.1.1', which are non-existent versions. The previous valid values were macos-15 and '16.2.0'/'16.3'. GitHub Actions currently supports macos-13, macos-14, and macos-15 runners, and Xcode 26.1.1 does not exist. Given the PR discussion comment "I need to stop..", these appear to be placeholder or test values that were accidentally committed. All CI jobs will fail until these are reverted to valid versions.
Additional Locations (1)
| entity.components = self.components.copy() | ||
| entity.components.entity = entity | ||
| entity.components.entity = entity.id | ||
| entity.components.world = world |
There was a problem hiding this comment.
Bug: Entity copy loses components due to unconditional didSet
The Entity.copy() method sets entity.components.world = world after copying components, but the didSet on ComponentSet.world unconditionally calls notFlushedComponents.removeAll(). This causes all copied components stored in notFlushedComponents to be lost. When copying an entity that hasn't been added to a world yet, the components are stored in notFlushedComponents, and this assignment clears them. For entities already in a world, the copied entity gets a world reference but uses notAllocatedId, making component lookups via world.get(from: entity) return nil since the world doesn't have that entity ID in its archetypes.
Additional Locations (1)
|
|
||
| public func update(context: inout UpdateContext) { | ||
| let result = self.fixedTimestep.advance(with: context.deltaTime) | ||
| @Res<DeltaTime?> |
There was a problem hiding this comment.
Bug: Fixed update schedulers not created but still referenced
The FixedTimeSchedulerSystem references .fixedPreUpdate, .fixedUpdate, and .fixedPostUpdate schedulers in its order array and tries to run them via world.runScheduler(). However, MainSchedulerPlugin.setup only adds .fixed and .postUpdate schedulers, removing the old code that created these three fixed schedulers. When FixedTimeSchedulerSystem.update executes, it calls runScheduler for schedulers that don't exist, causing Schedulers.getScope to log "Scheduler not found" errors and skip execution. Systems added to these fixed schedulers won't run unless users manually add systems that auto-create them.
Additional Locations (1)
| xcode_version: ['16.2', '16.3'] | ||
| runs-on: macos-15 | ||
| xcode_version: ['26.1.1'] | ||
| runs-on: macos-26 |
There was a problem hiding this comment.
Bug: GitHub Actions workflow uses non-existent macOS and Xcode versions
The GitHub Actions workflows specify macos-26 and xcode-version: '26.1.1' which don't exist. GitHub Actions only supports up to macos-15 and Xcode versions up to 16.x as of late 2025. This appears to be a typo where "26" was written instead of "16". The CI/CD pipelines will fail immediately because these runner images don't exist.
Additional Locations (1)
|
|
||
| // Update entity components | ||
| // entity.components += counter.wrappedValue | ||
| // entity.components += textComponent.wrappedValue |
There was a problem hiding this comment.
Bug: Performance counter text update disabled with print debugging
In PerformanceCounterSystem, the actual text rendering code is commented out (lines 377-378, 382, 385-386) while a print(text) debugging statement on line 381 remains active. The FPS counter will only output to console rather than displaying on screen. This appears to be debugging code that was accidentally left in.
| entity.components = self.components.copy() | ||
| entity.components.entity = entity | ||
| entity.components.entity = entity.id | ||
| entity.components.world = world |
There was a problem hiding this comment.
Bug: Entity copy loses components when not in world
When copying an entity that hasn't been added to a world, all its components are lost. The copy() method first copies the ComponentSet (which includes notFlushedComponents), but then immediately sets entity.components.world = world. This triggers the didSet observer on the world property, which unconditionally calls notFlushedComponents.removeAll(). Since entities not yet in a world store their components in notFlushedComponents, copying such entities results in a copy with no components.
| xcode_version: ['16.2', '16.3'] | ||
| runs-on: macos-15 | ||
| xcode_version: ['26.1.1'] | ||
| runs-on: macos-26 |
There was a problem hiding this comment.
Bug: CI uses non-existent macOS and Xcode versions
The workflow files reference macos-26 and xcode-version: '26.1.1', which don't exist. Current macOS versions max out at 15 (Sequoia), and Xcode at 16.x. The previous valid values were macos-15 and 16.2.0. These invalid values will cause all CI builds to fail immediately. Given the PR comment "I need to stop..", these appear to be accidental placeholder or typo values.
Additional Locations (1)
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Parallel forEach silently discards thrown errors
The forEach method is marked async rethrows and accepts a throwing operation, but uses await withThrowingTaskGroup instead of try await withThrowingTaskGroup. Since the body closure only adds tasks without throwing, and there's no iteration over group results (unlike the map method which correctly uses for try await), any errors thrown by the operation inside child tasks will be silently discarded rather than propagated to the caller.
|
|
||
| public func update(context: inout UpdateContext) { | ||
| let result = self.fixedTimestep.advance(with: context.deltaTime) | ||
| @Res<DeltaTime?> |
There was a problem hiding this comment.
Bug: Fixed-time schedulers referenced but never registered
FixedTimeSchedulerSystem attempts to run schedulers .fixedPreUpdate, .fixedUpdate, and .fixedPostUpdate, but these are never registered with the world. The MainSchedulerPlugin.setup only registers .main, .fixed, and .postUpdate schedulers. The old code explicitly added the fixed-time schedulers but this registration was removed during the refactor while the system still references them.
Additional Locations (1)
| entity.components = self.components.copy() | ||
| entity.components.entity = entity | ||
| entity.components.entity = entity.id | ||
| entity.components.world = world |
There was a problem hiding this comment.
Bug: Entity copy loses all components when setting world
The Entity.copy() method copies the ComponentSet including its notFlushedComponents, then sets entity.components.world = world. However, the world property has a didSet observer that calls self.notFlushedComponents.removeAll(). This means all copied components are immediately discarded after being copied, resulting in an empty entity. The copied entity loses all its component data.
Additional Locations (1)
| resource[keyPath: keyPath] = value | ||
| worlds.insertResource(resource) | ||
| let resource = worlds.main.getRefResource(T.self) | ||
| resource.wrappedValue[keyPath: keyPath] = value |
There was a problem hiding this comment.
Bug: updateResource crashes when resource doesn't exist in world
The updateResource modifier now calls getRefResource(T.self) which creates a Ref that can have a nil internal pointer when the resource doesn't exist. Accessing wrappedValue on this Ref force-unwraps the nil pointer, causing a crash. The previous implementation used a guard statement with getResource to safely handle missing resources. This regression will crash if updateResource is called before the resource is inserted into the world.
| // container.foregroundColor = .white | ||
|
|
||
| let text = "Bunnies: \(bunnyCount)\nFPS: \(String(format: "%.1f", counter.fps))" | ||
| print(text) |
There was a problem hiding this comment.
Bug: Debug print statement outputs every frame
The print(text) statement inside PerformanceCounterSystem.update outputs FPS and bunny count every single frame, which will flood the console during runtime. This appears to be debug code that was left in, as evidenced by the commented-out lines nearby that would properly update the text component instead.
| } | ||
|
|
||
| print() | ||
| } |
There was a problem hiding this comment.
Bug: Test code left in example application
The Kek component and TestPlugin struct appear to be debug/test code that was accidentally committed. The TestPlugin is commented out in the app initialization but the code still exists, including print() statements that would output test data. The PR author's comment "I need to stop.." suggests this may be work-in-progress code that shouldn't be committed.
|
|
||
| public var isEmpty: Bool { | ||
| commands.isEmpty | ||
| } |
There was a problem hiding this comment.
Bug: Data race in WorldCommandQueue isEmpty property
The isEmpty property and copy() method in WorldCommandQueue read commands without acquiring the lock, while push() uses locking. Additionally, applyAndDrop() mutates commands via popFirst() without locking. Since this class is marked @unchecked Sendable, these unprotected accesses can cause data races when the queue is accessed from multiple threads.
Additional Locations (2)
| self.buffer = other.buffer | ||
| self.bitset = other.bitset | ||
| self.entity = other.entity | ||
| self.notFlushedComponents = other.notFlushedComponents |
There was a problem hiding this comment.
Bug: Entity copy loses all components from world-bound entities
The ComponentSet.init(from:) copy constructor copies notFlushedComponents but doesn't copy the world reference. When an entity is added to a world, its components are flushed to archetype storage and notFlushedComponents is cleared (via the didSet on line 25). When Entity.copy() is called on a world-bound entity, the copied ComponentSet has no world reference and empty notFlushedComponents, resulting in all component data being lost.
Additional Locations (1)
| jobs: | ||
| build-and-deploy-docs: | ||
| runs-on: macos-15 | ||
| runs-on: macos-26 |
There was a problem hiding this comment.
Bug: GitHub Actions uses non-existent macos-26 runner image
The GitHub Actions workflow specifies runs-on: macos-26 which is not a valid runner image. The latest available macOS runner is macos-15. This will cause CI builds to fail as the runner cannot be provisioned.
Additional Locations (1)
| var container = TextAttributeContainer() | ||
| container.foregroundColor = .white | ||
|
|
||
| let text = unsafe "Bunnies: \(bunnyCount)\nFPS: \(String(format: "%.1f", counter.fps))" |
There was a problem hiding this comment.
Bug: Misplaced unsafe keyword before string literal
The unsafe keyword is incorrectly placed before a string literal expression (unsafe "Bunnies: ..."). In Swift's strict memory safety mode, unsafe is used to mark expressions involving pointer operations or other unsafe APIs, not regular string interpolation. This appears to be accidentally committed code that should be removed.
| container.foregroundColor = .white | ||
|
|
||
| let text = unsafe "Bunnies: \(bunnyCount)\nFPS: \(String(format: "%.1f", counter.fps))" | ||
| print(text) |
There was a problem hiding this comment.
Bug: Debug print statement left in production example code
A print(text) statement is called every frame in the PerformanceCounterSystem.update() method, logging bunny count and FPS to the console. This debug output will significantly impact performance and clutter logs in a stress test example meant to benchmark rendering performance.
Additional Locations (1)
| public extension Commands { | ||
| func append(_ commands: Commands) { | ||
| self.queue.commands.append(contentsOf: commands.queue.commands) | ||
| } |
There was a problem hiding this comment.
Bug: Race condition in WorldCommandQueue concurrent access
The WorldCommandQueue class uses a lock in push() for thread safety, but the append() method directly accesses self.queue.commands without acquiring the lock. Similarly, applyAndDrop(), copy(), and isEmpty don't use the lock. Since the class is marked @unchecked Sendable and push() is designed to be called from any thread, concurrent access between locked and unlocked operations on the internal Deque can cause data corruption or crashes.
|
|
||
| public var isLoaded: Bool { | ||
| self.asset == nil | ||
| } |
There was a problem hiding this comment.
Bug: isLoaded property returns inverted boolean value
The isLoaded property in AssetHandle returns self.asset == nil, which is the opposite of the intended meaning. When asset is nil, the asset is not loaded, so isLoaded would incorrectly return true. The condition is reversed and likely meant to be self.asset != nil.
| uses: maxim-lobanov/setup-xcode@v1.6.0 | ||
| with: | ||
| xcode-version: '16.2.0' | ||
| xcode-version: '26.1.1' |
There was a problem hiding this comment.
Bug: GitHub workflow uses non-existent macOS and Xcode versions
The CI workflows changed from macos-15 to macos-26 and Xcode from 16.2.0/16.3 to 26.1.1. Combined with the PR comment "I need to stop..", these appear to be accidental test/debug values. While macOS 26 and Xcode 26 may exist as future releases, using them in CI will likely cause workflow failures until these runners are available on GitHub Actions.
Additional Locations (1)
| self.queue.push { [entityId] world in | ||
| world.removeEntity(entityId) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: recursively parameter is ignored in removeFromWorld
The EntityCommands.removeFromWorld(recursively:) method accepts a recursively parameter but never uses it. The call to world.removeEntity(entityId) doesn't pass the recursively flag, so child entities will never be removed even when recursively: true is specified. This could cause orphaned entities or unexpected behavior when callers expect recursive removal.
|
|
||
| public var isLoaded: Bool { | ||
| self.asset == nil | ||
| } |
There was a problem hiding this comment.
Bug: Inverted logic in isLoaded property returns wrong value
The isLoaded computed property in AssetHandle returns self.asset == nil, which means it returns true when the asset is NOT loaded and false when it IS loaded. This is the opposite of what the property name suggests. The logic should be self.asset != nil to correctly indicate whether the asset has been loaded.
| uses: maxim-lobanov/setup-xcode@v1.6.0 | ||
| with: | ||
| xcode-version: '16.2.0' | ||
| xcode-version: '26.1.1' |
There was a problem hiding this comment.
Bug: GitHub CI uses non-existent macOS and Xcode versions
The CI workflows specify macos-26 runner and Xcode 26.1.1, which are non-existent versions. GitHub Actions runners currently support up to macos-15. These appear to be placeholder or future versions that were accidentally committed and will cause CI failures. The previous values were macos-15 and Xcode 16.2.0/16.3.
Additional Locations (1)
| container.foregroundColor = .white | ||
|
|
||
| let text = unsafe "Bunnies: \(bunnyCount)\nFPS: \(String(format: "%.1f", counter.fps))" | ||
| print(text) |
There was a problem hiding this comment.
Bug: Debug print statement left in performance counter system
The PerformanceCounterSystem contains a print(text) statement that outputs FPS and bunny count to the console on every frame when the counter updates. This debug statement will cause excessive console spam during runtime and should be removed from production code.
Additional Locations (1)
|
|
||
| print() | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Debug code "Kek" component and TestPlugin left in
The file contains debugging artifacts: a Kek component (with a non-descriptive name storing a World reference) and a TestPlugin that spawns 100 entities and prints debug output. These appear to be test code that was accidentally left in and should be removed before merging.
|
|
||
| public var isLoaded: Bool { | ||
| self.asset == nil | ||
| } |
There was a problem hiding this comment.
Bug: Inverted logic in isLoaded property check
The isLoaded computed property returns true when self.asset == nil, which is the opposite of the expected behavior. An asset handle is loaded when its asset is NOT nil. The condition should be self.asset != nil to correctly indicate that the asset has been loaded.
| xcode_version: ['16.2', '16.3'] | ||
| runs-on: macos-15 | ||
| xcode_version: ['26.1.1'] | ||
| runs-on: macos-26 |
There was a problem hiding this comment.
Bug: Invalid CI runner and Xcode version configuration
The GitHub Actions workflows reference macos-26 runner and Xcode version 26.1.1, which do not currently exist on GitHub Actions. The latest available macOS runner is macos-15, and while Xcode 26 betas may exist, the specific version 26.1.1 and the macos-26 runner image are not available, causing CI builds to fail.
Additional Locations (1)
| var container = TextAttributeContainer() | ||
| container.foregroundColor = .white | ||
|
|
||
| let text = unsafe "Bunnies: \(bunnyCount)\nFPS: \(String(format: "%.1f", counter.fps))" |
There was a problem hiding this comment.
Bug: Misplaced unsafe keyword before string literal
The unsafe keyword is incorrectly placed before a string literal (unsafe "Bunnies..."). In Swift, unsafe is used for memory operations and pointer access, not for string interpolation expressions. This appears to be accidentally committed code that will likely cause a compilation error or unexpected behavior.
| container.foregroundColor = .white | ||
|
|
||
| let text = unsafe "Bunnies: \(bunnyCount)\nFPS: \(String(format: "%.1f", counter.fps))" | ||
| print(text) |
There was a problem hiding this comment.
Bug: Debug print statement floods console every frame
A print(text) statement inside PerformanceCounterSystem.update() will execute every frame, flooding the console with bunny count and FPS output. This appears to be debug code that was accidentally left in the stress test example.
| self.asset = asset as! T | ||
| self.type = try container.decode(String.self, forKey: .type) | ||
| self.assetPath = try container.decode(String.self, forKey: .assetPath) | ||
| self.assetMeta = try container.decode(AssetMetaInfo.self, forKey: .meta) |
There was a problem hiding this comment.
Bug: Decoded AssetHandle has nil asset causing potential crashes
The init(from decoder:) method never initializes the asset property, leaving it nil. Since asset is declared as an implicitly unwrapped optional (T!), any code that decodes an AssetHandle and accesses asset without first checking isLoaded or calling load() will crash. The previous implementation ensured asset was always non-nil after construction.
Additional Locations (1)
| jobs: | ||
| build-and-deploy-docs: | ||
| runs-on: macos-15 | ||
| runs-on: macos-26 |
There was a problem hiding this comment.
Bug: CI uses non-existent macOS 26 GitHub Actions runner
The workflows specify runs-on: macos-26 and xcode-version: '26.1.1'. While macOS 26 (Tahoe) and Xcode 26 exist as Apple products, GitHub Actions macos-26 runner may not be available yet. The previous values (macos-15 and Xcode 16.2.0/16.3) were valid. CI builds will fail if this runner doesn't exist.
Additional Locations (1)
| while let drop = commands.popFirst() { | ||
| drop.applyToWorld(world) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Race condition in WorldCommandQueue due to inconsistent locking
WorldCommandQueue is marked @unchecked Sendable but has inconsistent thread safety. The push() method acquires the lock before modifying commands, but isEmpty, copy(), and applyAndDrop() all access commands without locking. If commands are pushed from one thread while being processed or read on another, this causes a data race on the Deque.
1 participant
Update logic for storing and accessing components
Note
Massive refactor introducing chunk-based ECS with async queries/systems, revamped schedulers/resources, updated render graph/pipelines and Metal backend, reworked input/UI, and added extensive tests.
Chunks,Chunk,ComponentLayout,BlobArray,SparseSet) and newArchetypemodel with edges.WorldQueryTarget,QueryTarget,FilterQuery,SystemParameter,Ref,Res/ResMut), change detection viaTick.SystemsGraph; addSystemasyncupdate.World(entities/records, command queue, resources API, extract/spawn APIs, change tracking).extract,prepare,render,renderRunner,fixedUpdateremoval/changes); newDefaultSchedulerRunner; async stage execution.CommandBuffer,RenderCommandEncoder,Blit*); refactor Metal backend (swapchain/drawables, windows), remove old draw list paths.Main2DRenderNode,UpscaleNode); introduce pipeline configurators/cache (RenderPipelines).RenderViewTarget, viewport/texture handling); window surfaces management in render world.InputbecomesResourcestruct with rumble engine, event clearing system; update Apple gamepad manager to emit via callback.UIRenderscaffolding and window management resources.BlobArray,SparseSet,UnsafeBox, fixed-array/set helpers, hashing/util extensions.Written by Cursor Bugbot for commit 0a8887e. This will update automatically on new commits. Configure here.